Repository navigation
fix(limits): avoid underflow panic when Rate resets race - #1105
Open
RaphaelFakhri wants to merge 1 commit into
Open
RaphaelFakhri wants to merge 1 commit into
RaphaelFakhri wants to merge 1 commit into
Conversation
Rate::maybe_reset subtracted timestamps without checking for underflow. When a thread read the clock before another thread stored a later reset time, `now - last_reset` underflowed. When a thread lost the reset race within the first second after the Rate was created, `now - 1000` underflowed and the sanity assert panicked, in both debug and release builds. Use saturating subtraction for both and add a concurrent test with a short interval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1104.
Rate::maybe_resetsubtracts timestamps without checking for underflow, so concurrent callers can panic:now - last_resetunderflows when another thread stores a later reset time after this thread reads the clock. In release builds the wrapped value also forces a spurious reset that clears both slots.now - 1000in the sanity assert underflows when a thread loses the reset race within the first second after theRateis created. This panics in both debug and release builds.This change:
saturating_subfor both subtractions. A thread that sees a newer reset time treats it as zero elapsed time and skips the reset.past_msfor the two-interval check instead of subtracting again.test_concurrent_reset_does_not_panic, which runs eight threads against aRatewith a 1 ms interval for 500 ms.Testing
Without the fix, the test fails on every run in debug and release builds. With the fix, it passes.
cargo test -p pingora-limits,cargo fmt --checkandcargo clippy -p pingora-limits --all-targets -- -D warningspass.